Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add muldelete action to TaskInstanceModelView and tests. Change permissions for TaskInstanceModelView.action_clear to "delete" #18438

Merged
merged 1 commit into from
Sep 30, 2021
Merged

Add muldelete action to TaskInstanceModelView and tests. Change permissions for TaskInstanceModelView.action_clear to "delete" #18438

merged 1 commit into from
Sep 30, 2021

Conversation

alex-astronomer
Copy link
Contributor

@alex-astronomer alex-astronomer commented Sep 22, 2021 •
edited
Loading

Closes #18333. Adds a Delete option in TaskInstanceModelView and deletes multiple TaskInstances based on which options are checked in the model view. Added unit tests for this as well. Tests that the right tasks get deleted and also tests that deletes get cascaded to TaskReschedules in the meta DB.

Also, changed permissions for DagRunModelView's action_clear to be edit instead of delete.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Sep 22, 2021
alex-astronomer
alex-astronomer commented Sep 22, 2021
View reviewed changes
tests/www/views/test_views_tasks.py Outdated
assert resp.status_code == 200
for task_search_tuple in task_search_tuples:
dag_id, task_id = task_search_tuple
session.query(TaskInstance).filter(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add an assert here... Adjusting now.

@alex-astronomer alex-astronomer force-pushed the 18333-muldelete-task-instances-model-view branch from acb0bf3 to 5e27c6f Compare September 22, 2021 17:27
bbovenzi
bbovenzi approved these changes Sep 23, 2021
View reviewed changes
Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work!

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Sep 23, 2021
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@Jorricks
Copy link
Contributor

There is a security issue here.
People will be able to delete TaskInstances that the person should not have access too.
You would just need to change the post values with a primary_key of another TaskInstance you don't have access too.
Therefore, please add the @action_has_dag_edit_access decorator.

@Jorricks Jorricks mentioned this pull request Sep 23, 2021
Merged
@Jorricks
Copy link
Contributor

People will be able to delete TaskInstances that the person should not have access too.

I created a PR to add a test to avoid the security issue in the future: #18467.
Please don't take my comment the wrong way, I appreciate the work you are doing and thing it is a clean implementation ✌️.
I should have written this test earlier and it's my mistake this wasn't made clear by the tests.

@alex-astronomer
Copy link
Contributor Author

People will be able to delete TaskInstances that the person should not have access too.

I created a PR to add a test to avoid the security issue in the future: #18467.
Please don't take my comment the wrong way, I appreciate the work you are doing and thing it is a clean implementation ✌️.
I should have written this test earlier and it's my mistake this wasn't made clear by the tests.

No offense taken, I'm happy that you commented because now I can learn something from this. I was wondering how I missed something like that since I just referred to the DagRun equivalent of the muldelete function but it looks like that decorator was added 2 days ago and I just wasn't up to date on my main branch. Good catch. I'm going to add that now.

@Jorricks
Copy link
Contributor

Nice, PR LGTM :)

@alex-astronomer
Copy link
Contributor Author

Any update on this one, merge team?

jedcunningham
jedcunningham reviewed Sep 29, 2021
View reviewed changes
airflow/www/views.py Outdated Show resolved Hide resolved
tests/www/views/test_views_tasks.py Outdated Show resolved Hide resolved
@alex-astronomer alex-astronomer force-pushed the 18333-muldelete-task-instances-model-view branch from fa75bac to 80841e7 Compare September 30, 2021 14:52
@alex-astronomer
Copy link
Contributor Author

@jedcunningham Changes addressed!

@Jorricks
Copy link
Contributor

@jedcunningham Changes addressed!

Nice work 👍

@jedcunningham jedcunningham added this to the Airflow 2.2.0 milestone Sep 30, 2021
@jedcunningham jedcunningham merged commit 932a225 into apache:main Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jorricks Jorricks Jorricks left review comments

@jedcunningham jedcunningham jedcunningham approved these changes

@bbovenzi bbovenzi bbovenzi approved these changes

@ashb ashb Awaiting requested review from ashb ashb is a code owner

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton ryanahamilton is a code owner

Assignees
No one assigned
Labels
area:webserver Webserver related Issues okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Milestone
Airflow 2.2.0
Development

Successfully merging this pull request may close these issues.

No Mass Delete Option for Task Instances Similar to What DAGRuns Have in UI
4 participants
@alex-astronomer @Jorricks @bbovenzi @jedcunningham